Skip to content

Add path annotation to apibindings#3691

Merged
kcp-ci-bot merged 1 commit intokcp-dev:mainfrom
mjudeikis:mjudeikis/add.path.annotation.to.bindings
Dec 6, 2025
Merged

Add path annotation to apibindings#3691
kcp-ci-bot merged 1 commit intokcp-dev:mainfrom
mjudeikis:mjudeikis/add.path.annotation.to.bindings

Conversation

@mjudeikis
Copy link
Contributor

@mjudeikis mjudeikis commented Oct 30, 2025 •
edited
Loading

Summary

This adds path annotation to apibindings. APIbindings are accessible via VirtualWorkspaces. This means providers will be able to resolve the canonical path and cluster more easily.

- apiVersion: apis.kcp.io/v1alpha2
  kind: APIBinding
  metadata:
    annotations:
      kcp.io/cluster: hezfu00lhrvcdnls
      kcp.io/path: root:test

Caveat: there is a race condition with logicalcluster. But it resolves quite quickly due to the binding handshake, which is followed by the update. Not ideal, but the alternatives (the ones I could think of) were even more hacky.

What Type of PR Is This?

/kind feature

Related Issue(s)

Fixes #3673

Release Notes

Add kcp.io/path annotation to APIBindings

@kcp-ci-bot kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. dco-signoff: yes Indicates the PR's author has signed the DCO. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Oct 30, 2025
@mjudeikis mjudeikis changed the title add path annotation to apibindings Add path annotation to apibindings Oct 30, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/add.path.annotation.to.bindings branch from 0ce8c8a to 1e17495 Compare October 30, 2025 10:40
@kcp-ci-bot kcp-ci-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/add.path.annotation.to.bindings branch 3 times, most recently from be46ed4 to d10aecd Compare October 30, 2025 12:16
@mjudeikis mjudeikis marked this pull request as draft October 30, 2025 12:33
@kcp-ci-bot kcp-ci-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2025
@mjudeikis
Copy link
Contributor Author

mjudeikis commented Oct 30, 2025

moving to draft. some strange errors pop up. Investigating.

@mjudeikis
Copy link
Contributor Author

mjudeikis commented Oct 30, 2025

/test all

@mjudeikis mjudeikis force-pushed the mjudeikis/add.path.annotation.to.bindings branch 2 times, most recently from b7a09dc to bcfb633 Compare October 30, 2025 13:29
@mjudeikis mjudeikis marked this pull request as ready for review October 30, 2025 13:30
@kcp-ci-bot kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/add.path.annotation.to.bindings branch from bcfb633 to 4cadc05 Compare November 5, 2025 10:32
@mjudeikis
Copy link
Contributor Author

mjudeikis commented Nov 5, 2025

/retest

@gman0
Copy link
Contributor

gman0 commented Nov 5, 2025

Another approach would be to:

  1. Skip mutating/validating objects whose logical cluster resolves to NotFound:
    • The reasoning would be that it's probably not a job of this plugin to deny requests with missing LogicalCluster -- something else should catch and validate this if we really need such a check.
  2. Modify the APIBinding admission plugin to add an empty kcp.io/path annotation on the object if it's missing during create/update.
    • The pathannotation admission plugin would then catch this, and update the path accordingly once the APIBinding's logical cluster is ready.

Maybe this divides the responsibilities a bit better? But it may be that I'm missing some knowledge about why it is done like that in the first place :P

@gman0
Copy link
Contributor

gman0 commented Nov 5, 2025

Also, existing APIBindings are not updated right? Is that a problem?

@mjudeikis
Copy link
Contributor Author

mjudeikis commented Nov 5, 2025

Another approach would be to:

  1. Skip mutating/validating objects whose logical cluster resolves to NotFound:

    • The reasoning would be that it's probably not a job of this plugin to deny requests with missing LogicalCluster -- something else should catch and validate this if we really need such a check.

  2. Modify the APIBinding admission plugin to add an empty kcp.io/path annotation on the object if it's missing during create/update.

    • The pathannotation admission plugin would then catch this, and update the path accordingly once the APIBinding's logical cluster is ready.

Maybe this divides the responsibilities a bit better? But it may be that I'm missing some knowledge about why it is done like that in the first place :P

So my code is attempting to do option 2:
But I didn't want to do an empty annotation as some of the other code relies on this, and empty is invalid. So skipping kinda looked like a better alternative.

And it still gets updated/added once a logical cluster is added and the next resync/ status update.

@gman0
Copy link
Contributor

gman0 commented Nov 5, 2025

I agree 👍

/lgtm

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2025
@kcp-ci-bot
Copy link
Contributor

kcp-ci-bot commented Nov 5, 2025

LGTM label has been added.

DetailsGit tree hash: ce0a2111a22aca51e986c418f7d76cd7090bf820

@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2025
@mjudeikis mjudeikis closed this Dec 2, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/add.path.annotation.to.bindings branch from 4cadc05 to 1736b49 Compare December 2, 2025 14:45
@kcp-ci-bot kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2025
@kcp-ci-bot kcp-ci-bot requested a review from gman0 December 2, 2025 14:45
@kcp-ci-bot kcp-ci-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 2, 2025
@mjudeikis mjudeikis deleted the mjudeikis/add.path.annotation.to.bindings branch December 2, 2025 14:47
@mjudeikis mjudeikis restored the mjudeikis/add.path.annotation.to.bindings branch December 2, 2025 14:47
@mjudeikis
Add path annotation to apibindings
2448833 
Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt>
On-behalf-of: @SAP mangirdas.judeikis@sap.com
@mjudeikis mjudeikis reopened this Dec 2, 2025
@kcp-ci-bot kcp-ci-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 2, 2025
@mjudeikis
Copy link
Contributor Author

mjudeikis commented Dec 3, 2025

/retest

@gman0
Copy link
Contributor

gman0 commented Dec 3, 2025

/lgtm

We know the limitations, but it's better than the alternatives. And the limitations don't seem to be very serious.

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2025
@kcp-ci-bot
Copy link
Contributor

kcp-ci-bot commented Dec 3, 2025

LGTM label has been added.

DetailsGit tree hash: 2ddcde036236088f0320e92a0d243108434f2132

@xrstf
Copy link
Contributor

xrstf commented Dec 5, 2025

For my own understanding: For just resolving the cluster ID to the workspace path, one could (like the syncagent does) simply also claim logicalclusters, right?

/approve

@kcp-ci-bot
Copy link
Contributor

kcp-ci-bot commented Dec 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xrstf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2025
@xrstf
Copy link
Contributor

xrstf commented Dec 5, 2025

This can be done already if one claims LogicalCluster, but this elevated apibinding/export contract quite high.

Yeah you said that already in the ticket. Nevermind. ^^

@xrstf
Copy link
Contributor

xrstf commented Dec 5, 2025

/retest

2 similar comments
@olamilekan000
Copy link
Contributor

olamilekan000 commented Dec 5, 2025

/retest

@xrstf
Copy link
Contributor

xrstf commented Dec 6, 2025

/retest

@kcp-ci-bot kcp-ci-bot merged commit 7788cc5 into kcp-dev:main Dec 6, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gman0 gman0 Awaiting requested review from gman0

Assignees

@gman0 gman0

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feature: add kcp.io/path to apibindings

5 participants

@mjudeikis @gman0 @kcp-ci-bot @xrstf @olamilekan000